Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Garden] Download models in the background #1484

Closed
wants to merge 83 commits into from
Closed

[Garden] Download models in the background #1484

wants to merge 83 commits into from

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 13, 2022 •
edited by chapulina
Loading

🎉 New feature

Summary

it requires gazebosim/sdformat#1027

I was trying to add this functionality to Citadel, but we need to load a simple world when we are downloading the models, this simple world will add a name to the Scene which in most of the cases is not going to match with the desired world name.

This will generate that SceneBroadcaster will create some topics with undesired names. Using the reset API I can create the desired topics but, the old topics are still there.

Example:

/clock
/gazebo/resource_paths
/gui/camera/pose
/stats
/world/empty_world/clock
/world/empty_world/dynamic_pose/info
/world/empty_world/pose/info
/world/empty_world/scene/deletion
/world/empty_world/scene/info
/world/empty_world/state
/world/empty_world/stats
/world/shapes/dynamic_pose/info
/world/shapes/pose/info
/world/shapes/scene/deletion
/world/shapes/scene/info
/world/shapes/state

Another issue happens in the Physics thread, sometimes some links are not added. Myabe a race condition ?

It builds on top of #1327

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

mjcarroll and others added 30 commits February 21, 2022 13:56
@mjcarroll
SystemInternal and WorldControl into own headers
11e029a 
Reduces the size of simulationrunner header a bit

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Move System interaction to SystemManager
56f28bd 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Adjustments for Fortress
c35e3ce 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Address reviewer feedback
8266e73 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll @azeey
Introduce System::Reset API
ca91023 
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll @azeey
SimulationRunner Reset implementation
85cb36e 
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll @azeey
Simulation Reset implementation in Physics system
144556d 
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll @azeey
Simulation reset example plugin
4b023fa 
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Add integration test for resetting physics
75e57fb 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Fix diff test and add documentation
e3368e5 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@azeey
Fix build and test errors after merge, some documentation and cleanup.
63ec622 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Merge branch 'reset' into reset_physics
222695d 
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@mjcarroll
Merge branch 'main' into reset_physics
e5f01bc
@mjcarroll
Bump year
859df32 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Merge branch 'main' into reset_physics
8c669b0
@mjcarroll
Fix merge from main
749daca 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Fix CI platform
371d8ce 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll @azeey
Update test/integration/reset.cc
d1fabbe 
Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
@mjcarroll
Merge branch 'main' into reset_physics
8ddb739
@chapulina
Merge branch 'main' into reset_physics
32d21f5 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@mjcarroll
Merge branch 'main' into reset_physics
a766933
@mjcarroll
Address reviewer feedback
c8ab4d5 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Reset sensors and scenebroadcaster implementation
0c52ab3 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Remove debug outputs
825c280 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Address reviewer feedback
68f6f85 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Merge branch 'main' into reset_physics
0bd1d6b
@mjcarroll
Adjust randomized range to account for limits
3799ff3 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Fix header for new location
fbf6d0e 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Merge branch 'reset_physics' into reset_sensors
383006f
@mjcarroll
Merge branch 'main' into reset_physics
5cff795
@mjcarroll
Merge branch 'main' into ahcorde/garden/download_parallel
ff6d575 
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Copy link
Contributor

I think that the merge is okay. It may be cleaner to rebase on main at this point, though?

@ahcorde
Reduce diff
45c64c6 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Reduce diff
b9b015b 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Reduce diff
2ca8584 
Signed-off-by: ahcorde <ahcorde@gmail.com>
mjcarroll
mjcarroll reviewed Jun 30, 2022
View reviewed changes
src/SystemManager.cc Outdated Show resolved Hide resolved
ahcorde and others added 3 commits July 7, 2022 14:33
@ahcorde
Added logic to remove system plugins
836cd82 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
make linters happy
d0ae285 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@mjcarroll
Merge branch 'main' into ahcorde/garden/download_parallel
6873c32
@mjcarroll
Copy link
Contributor

This seems odd, did it come in from the merge from main?

/github/workspace/src/Server.cc: In member function 'void gz::sim::v7::Server::Init()':
  /github/workspace/src/Server.cc:207:43: error: 'class sdf::v13::Root' has no member named 'WorldNameFromFile'
    207 |     auto errors2 = this->dataPtr->sdfRoot.WorldNameFromFile(

@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 8, 2022

This seems odd, did it come in from the merge from main?

/github/workspace/src/Server.cc: In member function 'void gz::sim::v7::Server::Init()':
  /github/workspace/src/Server.cc:207:43: error: 'class sdf::v13::Root' has no member named 'WorldNameFromFile'
    207 |     auto errors2 = this->dataPtr->sdfRoot.WorldNameFromFile(

@mjcarroll This is not merged yet gazebosim/sdformat#1027

@mjcarroll
Copy link
Contributor

@mjcarroll This is not merged yet gazebosim/sdformat#1027

Ah, sorry didn't catch that. Looks like it has an approval, do you want to merge/release or wait for @chapulina ?

@chapulina
Copy link
Contributor

do you want to merge/release or wait for @chapulina ?

Don't wait for me on the SDF PR 🚢

@ahcorde
update method name
29e0a8a 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Merge branch 'ahcorde/garden/download_parallel' of https://github.com…
46e707c 
…/ignitionrobotics/ign-gazebo into ahcorde/garden/download_parallel
jennuine
jennuine reviewed Jul 13, 2022
View reviewed changes
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an initial pass

examples/worlds/fuel.sdf Outdated Show resolved Hide resolved
examples/worlds/fuel.sdf Show resolved Hide resolved
src/SdfEntityCreator.cc Outdated Show resolved Hide resolved
src/Server.cc
Comment on lines +89 to +91
errors = root.LoadSdfString(
this->dataPtr->config.SdfString());
this->dataPtr->sdfRoot = root.Clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use this->dataPtr->sdfRoot directly?

Suggested change
errors = root.LoadSdfString(
this->dataPtr->config.SdfString());
this->dataPtr->sdfRoot = root.Clone();
errors = this->dataPtr->sdfRoot.LoadSdfString(
this->dataPtr->config.SdfString());

Below as well

src/Server.cc
errors = root.Load(filePath);
this->dataPtr->sdfRoot = root.Clone();

if (!this->dataPtr->config.DownloadInParallel())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does DownloadInParallel get set to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in src/gz.cc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it get set to true but I never see if it get set to false

src/Server.cc Outdated Show resolved Hide resolved
src/SimulationRunner.hh Outdated Show resolved Hide resolved
src/SystemManager.cc Outdated Show resolved Hide resolved
include/gz/sim/ServerConfig.hh Outdated Show resolved Hide resolved
src/SdfEntityCreator.cc Show resolved Hide resolved
@chapulina chapulina added bug Something isn't working and removed needs upstream release Blocked by a release of an upstream library labels Jul 25, 2022
@chapulina chapulina changed the base branch from main to gz-sim7 August 10, 2022 18:20
merged with gz-sim7
a89161d 
Signed-off-by: Nate Koenig <nate@openrobotics.org>
@nkoenig nkoenig mentioned this pull request Aug 26, 2022
Closed
9 tasks
@nkoenig
Copy link
Contributor

nkoenig commented Aug 26, 2022

@ahcorde see #1669 as an alternative that doesn't need the restart feature.

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@azeey
Copy link
Contributor

azeey commented Aug 18, 2023

Closing in favor of #1669

@azeey azeey closed this Aug 18, 2023
@azeey azeey removed the beta Targeting beta release of upcoming collection label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennuine jennuine jennuine left review comments

@mjcarroll mjcarroll mjcarroll left review comments

@chapulina chapulina Awaiting requested review from chapulina

@iche033 iche033 Awaiting requested review from iche033

@nkoenig nkoenig Awaiting requested review from nkoenig

Assignees

@ahcorde ahcorde

Labels
bug Something isn't working 🌱 garden Ignition Garden
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Download Fuel models on the background
7 participants
@ahcorde @xlla @jennuine @mjcarroll @chapulina @nkoenig @azeey